Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add JNI code for ed25519-zebra #37

Merged
merged 10 commits into from
Feb 26, 2021
Merged

Add JNI code for ed25519-zebra #37

merged 10 commits into from
Feb 26, 2021

Conversation

droark
Copy link
Contributor

@droark droark commented Jan 7, 2021

Add some code allowing other languages, via JNI, to interact with ed25519-zebra. The initial commit:

  • Allows users to obtain a random 32 byte signing key seed.
  • Allows users to obtain a 32 byte verification key from a signing key seed.
  • Allows users to sign arbitrary data.
  • Allows users to verify an Ed25519 signature.
  • Includes a Java file that can be used.
  • Includes some Scala-based JNI tests.

@droark droark force-pushed the main branch 13 times, most recently from 137c57b to 4183cc7 Compare January 27, 2021 07:40
Add JNI code for ed25519-zebra
08b2366 
Add some code allowing other languages, via JNI, to interact with ed25519-zebra. The initial commit:

- Allows users to obtain a random 32 byte signing key seed.
- Allows users to obtain a 32 byte verification key from a signing key seed.
- Allows users to sign arbitrary data.
- Allows users to verify an Ed25519 signature.
- Includes a Java file that can be used.
- Includes some Scala-based JNI tests.
@yaahc yaahc requested review from yaahc and dconnolly January 27, 2021 19:41
yaahc
yaahc reviewed Jan 27, 2021
View reviewed changes
ed25519jni/.cargo/config Outdated Show resolved Hide resolved
ed25519jni/Cargo.toml Outdated Show resolved Hide resolved
ed25519jni/src/main/rust/lib.rs Outdated Show resolved Hide resolved
ed25519jni/src/main/rust/utils/exception.rs Outdated Show resolved Hide resolved
ed25519jni/src/main/rust/lib.rs Outdated Show resolved Hide resolved
Review fixups
dba6479 
- Minor Rust code optimizations.
- Rust build optimizations.
- Tweak the JNI JAR prereq script to match the new outputs.
@droark droark requested a review from yaahc January 27, 2021 21:03
@droark
Copy link
Contributor Author

droark commented Jan 27, 2021 •
edited
Loading

@yaahc - Folded in your requested changes. The build system might not be fully optimized, though. I noticed that libed25519jni.a and libed25519jni.{so/dylib} appear in both target/{mode} and target/{mode}/deps. Is there a way to get those files to appear only once? I'm assuming it's something subtle that I missed.

Thanks.

@droark
Copy link
Contributor Author

droark commented Jan 28, 2021

@yaahc - Pushed some more build system changes. They partially undid your requested changes, such as removing the workspace. If you think it's worthwhile to put that back, I'm happy to discuss it. Things are fine on my end but I don't mind moving things around if they'll improve things or fit standard Rust patterns.

Significant cleanup
fd35083 
- More build system tidying. The primary goal is to try to firewall the JNI code from everything else.
- README tidying.
Douglas Roark added 2 commits January 31, 2021 22:24
Grab bag of improvements
39b96e8 
- Clean up the wrapper classes (streamlining, make constructors private, more mutability safety).
- private -> public for a static variable intended for public usage.
- Minor comment & build system cleanup.
Bump JNI version to 0.0.4-DEV
3685945 
Decided to bump the version to reflect earlier changes.
yaahc
yaahc reviewed Feb 2, 2021
View reviewed changes
ed25519jni/rust/Cargo.toml Outdated Show resolved Hide resolved
@yaahc
Copy link

yaahc commented Feb 2, 2021 •
edited
Loading

@yaahc - Pushed some more build system changes. They partially undid your requested changes, such as removing the workspace. If you think it's worthwhile to put that back, I'm happy to discuss it. Things are fine on my end but I don't mind moving things around if they'll improve things or fit standard Rust patterns.

oh, so you removed the workspace but didn't add back the .cargo/config. That's probably fine, just need to be careful to make sure our CI still runs the tests in the subcrate. Out of curiosity what was the issue with the workspace that led you to remove it?

@droark
Copy link
Contributor Author

droark commented Feb 4, 2021

@yaahc - I'm a little confused. I was under the impression that .cargo/config wasn't necessary. It also seemed to be messing with the path within the target directory.

Regarding why it was added in the first place, it's a bit of cruft. At first, the Java and the Rust code were basically sitting on top of each other. The config file got added in order to have a place where target files could go. Once everything got separated and worked out on my end, the file didn't seem to be necessary anymore. If it's causing CI problems, I can definitely reassess.

Douglas Roark added 2 commits February 4, 2021 12:49
Hard-code the ed25519-zebra version for ed25519jni to use
0bc2829
Unify ed25519 JNI version
67cd715 
Also add "-JNI" to assist with tagging and otherwise distinguish the JNI code from the main library version/code.
@yaahc
Copy link

yaahc commented Feb 4, 2021

@yaahc - I'm a little confused. I was under the impression that .cargo/config wasn't necessary. It also seemed to be messing with the path within the target directory.

Oh, you're right, it isn't necessary. I thought you'd used it to deduplicate the target/ directory between the inner and the outer crate so they can share compilation artifacts. This is why I suggested switching to a workspace, because one of the features of a workspace is that all crates in the workspace share a single target directory. So when you removed both I was confused because I assumed this would cause issues with target/ directories for you that had been a problem before, but it seems that is not the case, so it's fine.

Regarding why it was added in the first place, it's a bit of cruft. At first, the Java and the Rust code were basically sitting on top of each other. The config file got added in order to have a place where target files could go. Once everything got separated and worked out on my end, the file didn't seem to be necessary anymore. If it's causing CI problems, I can definitely reassess.

👍

Douglas Roark added 3 commits February 22, 2021 14:24
Add code to make VerificationKeyBytes comparison easier
693c843 
Also add a test suite for VerificationKeyBytes.
VerificationKeyBytes cleanup
12b94e8 
- Fix hashCode() override.
- Add a test.
- Remove unneecessary semicolons.
Add Signature to JNI
bf06db8 
Mirror the Signature struct from Rust and add some basic tests. Also do a bit of Scala test cleanup.
@droark droark requested a review from yaahc February 26, 2021 18:54
@droark
Copy link
Contributor Author

droark commented Feb 26, 2021

@yaahc - Okay, I think we're finally golden. :) The last 2-3 commits should be all that you need to review but I understand if you want to review the whole thing again. My apologies for always adding stuff. This should be it for awhile.

@yaahc yaahc merged commit 2abe8b9 into ZcashFoundation:main Feb 26, 2021
@teor2345 teor2345 mentioned this pull request Apr 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaahc yaahc yaahc approved these changes

@dconnolly dconnolly Awaiting requested review from dconnolly

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@droark @yaahc